Repository navigation
feat(sync): add the TypeScript and JavaScript adapters - #992
Conversation
1358e52 to
bea1e3f
Compare
bea1e3f to
5c0370a
Compare
5c0370a to
6312205
Compare
6312205 to
c022ee0
Compare
2301e27 to
db9ebdd
Compare
db9ebdd to
33c5104
Compare
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
TypeScript export-equals is ignored
Medium Severity
import x = require(...) is bound as a namespace and as the default, but the matching TypeScript export export = value is never read. typeScriptDefaultExport only looks for export default, and typeScriptExportedNames only records export const / export { ... } and module.exports. A CJS file that uses export = TEXT or export = { TEXT } therefore has no importable constants, so a require or import-equals of that module stays unresolved.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ddadedd. Configure here.
There was a problem hiding this comment.
Fixed. typeScriptModuleExports reads export = value through typeScriptExportEquals as it reads module.exports = value: an object exports its entries, an identifier or a literal exports the whole module, and a literal declares the default. typeScriptConstants now collects these exports before it handles export default. TestTypeScriptExportEqualsIsImportable covers export = TEXT, export = { TEXT }, and export = "text" through import x = require(...).
| if previous, seen := instances[id]; seen && previous != kind { | ||
| ambiguous[id] = true | ||
| } | ||
| instances[id] = kind |
There was a problem hiding this comment.
Constructor this-assignment drops client kind
Medium Severity
Assigning a constructor parameter onto this, such as this.client = client, records an empty instance kind and can wipe a real client binding. JavaScript has no parameter properties, so the usual constructor pattern never tags this.client, and later this.client.chat.completions.create(...) (and the Anthropic equivalent) is missed. TypeScript only works when the parameter uses an accessibility modifier.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4b811ce. Configure here.
There was a problem hiding this comment.
Fixed. When a constructor stores a parameter on this, typeScriptInstances reads the type of the parameter: client: OpenAI tags the property with the client kind, and a parameter without a type records nothing, so it does not wipe the kind that a field gave the property. TestTypeScriptConstructorParametersKeepTheClientKind covers a typed TypeScript parameter and a plain JavaScript parameter over a field.
| } | ||
| if frozen != nil { | ||
| environment[name], values[name] = frozen, frozen | ||
| } |
There was a problem hiding this comment.
Namespace mutation misses named imports
Medium Severity
A write through a namespace import records values.LIST, and the freeze pass only marks that namespace field. A named import of the same export stays writable, and the reverse is also true. At runtime those bindings are one value, so sync can still emit edits for an export that the file already mutated.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9a6741a. Configure here.
There was a problem hiding this comment.
Fixed. typeScriptSpreadImportMutations links a named import and the field of a namespace import that hold one export, by the shared value, and spreads a mutation through either name to both before the freeze pass. TestTypeScriptNamedAndNamespaceImportsShareMutations covers a push through the named import, a push through the namespace, and an untouched export that stays writable through both names.
| // scope. The marker counts as a class scope, so a bare name | ||
| // does not reach the static fields through it. | ||
| entry = append([]typeScriptScope{{name: "static", class: true}}, entry...) | ||
| } |
There was a problem hiding this comment.
Static field scopes omit static marker
Medium Severity
The static scope marker is applied only to method_definition, so a static field initializer shares ids with an instance field of the same name. Locals in a static field arrow are treated as reassigned, and agents or tools stored on the field get positional ids instead of a stable Class.static.field key.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c5bc94d. Configure here.
There was a problem hiding this comment.
Fixed. typeScriptStaticScope applies the static marker to a static method and to a function or class that initializes a static field, so a static field arrow has the scope Class.static.field and an instance field of the same name is another scope. TestTypeScriptStaticFieldInitializersAreScopesOfTheClass covers a static and an instance field arrow with one name and one local.
| name, value := typeScriptBinding(node) | ||
| if name == nil || value == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Typed parameter properties skip client kinds
Medium Severity
constructor(private client: OpenAI) never records a client kind. typeScriptBinding returns that parameter with no value, and typeScriptInstances then drops every nil value before typeScriptParameterKind can read the annotation. Methods that call this.client.chat.completions.create(...) or this.client.messages.create(...) are therefore not collected. The covered form is only a parameter property with a new OpenAI() default, which is not the usual TypeScript injection pattern.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 45f1733. Configure here.
There was a problem hiding this comment.
Fixed. typeScriptInstances records a parameter property without a value, such as constructor(private client: OpenAI), with the kind that its type names, through typeScriptParameterKind, so this.client.chat.completions.create(...) in a method is collected. TestTypeScriptTypedParameterPropertiesAreClients covers an OpenAI and an Anthropic parameter property next to a string one.
| // default import binds it too. | ||
| if visible["default"] == nil && visible["*"] != nil { | ||
| visible["default"] = visible["*"] | ||
| } |
There was a problem hiding this comment.
CJS object export misses default import
Medium Severity
A default import of a CommonJS module that assigns an object to module.exports (or export = { ... }) stays unresolved. Named keys become exports, but the object itself is never recorded as * or default, so the interop fallback never runs. require and import * as still work; import x from "./mod" and x.field do not, which is the usual esModuleInterop shape.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2480e2e. Configure here.
There was a problem hiding this comment.
Fixed. module.exports = { ... } and export = { ... } mark the object as the module, so the import resolution binds the named exports as one object for the * binding and the default import, as require already did. An entry named default replaces the marker. TestTypeScriptDefaultImportOfACommonJSObjectModule covers both forms through import x from and x.field.
| return instances[strings.Join(append(typeScriptMemberScopes(node, source), field), ".")] | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Static clients ignored on class name
Medium Severity
A static client field is stored as Class.static.client, and this.client inside a static method finds it. A call through the class name, such as Class.client.chat.completions.create(), never inserts the static marker, so the receiver has no kind and the request is dropped. Constant reads and mutations through that same class name already insert the marker.
Reviewed by Cursor Bugbot for commit 2480e2e. Configure here.
There was a problem hiding this comment.
Fixed. typeScriptInstanceKind tries the receiver with the static marker after the class name, Bot.static.client for Bot.client, as a constant read through the class name does, so a request through the class name finds the static client inside and outside the class. TestTypeScriptStaticClientsResolveThroughTheClassName covers both.
| return "" | ||
| } | ||
| return imports.at(use, source)[code.Text(named, source)] | ||
| } |
There was a problem hiding this comment.
Parameter types miss type-only imports
Medium Severity
typeScriptParameterKind looks up a parameter’s type in the value-import map, which skips import type and { type X } bindings, and it only accepts a bare identifier. Typed parameter properties and this.client = client therefore get no client kind for import type OpenAI from "openai" or client: OpenAI | undefined, so this.client.chat.completions.create(...) is not collected. Those forms are the usual TypeScript shape under verbatimModuleSyntax.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2cc09c6. Configure here.
There was a problem hiding this comment.
Fixed. The instances read parameter types through typeScriptTypeImports, which keeps the type-only bindings of import type and { type X } that the value imports leave out, and typeScriptParameterKind finds the one type of the annotation that an import binds, so client: OpenAI | undefined and client?: OpenAI name the kind. TestTypeScriptTypeOnlyImportsNameParameterKinds covers a type-only default import with a union type and a type-only named import stored on this.
| id := typeScriptBindingID(node, code.Text(name, source), source) | ||
| declarations = append(declarations, declaration{id: id, value: valueNode}) | ||
| counts[id]++ | ||
| }) |
There was a problem hiding this comment.
Locals fail to shadow outer constants
Medium Severity
typeScriptConstants only records a variable when it is a simple identifier with an initializer, so destructuring, for/of targets, and uninitialized let never hide an outer constant of the same name. A use of that name then reads the import or module binding instead of the local, and a write can edit the wrong file. Parameters already get a nil shadow, and the import-alias walk already uses bound names for these forms.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4a3d440. Configure here.
There was a problem hiding this comment.
Fixed. The declaration walk records a destructuring, an uninitialized let or const, and a for-of or for-in target as bindings without a value, in their block, so a use of the name reads the local instead of an outer constant or import. TestTypeScriptLocalsOfEveryFormHideOuterConstants covers all three forms next to a module use that still resolves.
| if !inClass { | ||
| return typeScriptScopes(node, source) | ||
| } | ||
| return scopes |
There was a problem hiding this comment.
Anonymous classes break this.client lookup
Medium Severity
typeScriptClassScopes skips a class with no scope name before it can mark inClass, so this.client in a method of an anonymous class is keyed under the method while the field client = new OpenAI() is not. Mixin factories and export default class { ... } then miss the client and drop the request.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 74de14c. Configure here.
There was a problem hiding this comment.
Fixed. typeScriptScopeName gives an anonymous class a name of its own from its position, so typeScriptClassScopes marks the class and a field and a method of export default class {} or of a mixin class share one scope, and this.client finds the field. TestTypeScriptAnonymousClassesKeepTheirClients covers a default export class and a mixin factory.
| } | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Class name resolves instance over static
High Severity
typeScriptInstanceKind looks up Bot.client as a bare scoped name before trying Bot.static.client. An instance field uses the id Bot.client, so that hit wins and the static fallback never runs. In JavaScript, Bot.client is the static member; the instance field is this.client. A request through the class name can bind the instance client, or miss the static one, whenever both fields share a name. Constant reads and mutation tracking already try the static marker first.
Reviewed by Cursor Bugbot for commit f38c0d4. Configure here.
There was a problem hiding this comment.
Fixed. typeScriptInstanceKind tries the static marker first for a dotted receiver whose root is not this, so Bot.client reads the static field even when an instance field has the same name, and this.client reads the instance field, as a constant read through the class name already did. TestTypeScriptClassNameReadsTheStaticClientFirst covers a class with both fields.
| } | ||
| } | ||
| } | ||
| record(typeScriptInstanceID(node, name, source), kind) |
There was a problem hiding this comment.
Identifier assign wipes client kind
Medium Severity
typeScriptInstances still records an empty kind for an identifier assignment that is not a parameter. That empty kind disagrees with an existing client kind, so record marks the id ambiguous and the cleanup deletes it. this.client = local or client = backup then drops a client that a field or earlier new already established, and later framework calls on that name are skipped. Null, undefined, and untyped parameters already return without recording to avoid this wipe.
Reviewed by Cursor Bugbot for commit f38c0d4. Configure here.
There was a problem hiding this comment.
Fixed. An identifier assignment that is not a parameter takes the kind of the assigned name: a name with a kind moves it, and a name without one records nothing, so this.client = kept does not wipe the kind of the field, while two kinds on one name still make it ambiguous. TestTypeScriptIdentifierAssignmentsKeepOrMoveTheClientKind covers a name without a kind on a field and a name with a kind on an uninitialized let.
| record(typeScriptInstanceID(node, name, source), kind) | ||
| } | ||
| } | ||
| return |
There was a problem hiding this comment.
Typed parameters miss client instances
Medium Severity
A typed parameter such as function run(client: OpenAI) never becomes an instance. typeScriptBinding only returns a parameter that is a parameter property or has a default, and the nil-value branch of typeScriptInstances only reads types for parameter properties. typeScriptParameterKind and type-only imports therefore never tag a normal function or method parameter, so client.chat.completions.create(...) inside that function is not collected.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f38c0d4. Configure here.
There was a problem hiding this comment.
Fixed. A required or optional parameter without a default, such as function run(client: OpenAI) or ask(client?: Anthropic), becomes an instance of its function through the type of the parameter, read through the type imports, and a parameter property stays a property of the instance. TestTypeScriptTypedParametersAreClients covers a function parameter with a type-only import and an optional method parameter.
| // The kind of that name decides, and a name without one | ||
| // does not change the kind that the property holds. | ||
| return | ||
| } |
There was a problem hiding this comment.
Locals fail to shadow outer clients
Medium Severity
typeScriptInstances comments that a non-client binding still gets an empty entry so a local shadows an outer client, but several common bindings never record one. An untyped parameter returns without an entry, and an identifier right-hand side with no kind returns to avoid wiping this.client. typeScriptBinding also skips uninitialized lets, destructuring, for-of targets, and JavaScript identifier parameters. A later client.chat.completions.create(...) then resolves to the outer OpenAI instance, so the adapter can collect and edit the wrong call.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9f3673c. Configure here.
There was a problem hiding this comment.
Fixed. typeScriptInstances declares an entry without a kind for every binding that has no value: an untyped parameter, a JavaScript identifier or destructured parameter, a catch parameter, a local function or class, an uninitialized let, a destructuring, and a for-of target, and a destructuring or a local bound to a name without a kind declares one too, so a later client.chat.completions.create(...) resolves to the local and not to an outer client. Such an entry is unset: a later assignment gives the name its kind without making it ambiguous, so let client; client = new OpenAI() still works, and this.client = other still keeps the kind of the field. TestTypeScriptLocalsShadowOuterClients covers the five local forms next to a module use that still resolves.
Add the TypeScript and JavaScript adapters with the same static rules as Python. Add the package that registers every shipped format, and the conformance tests that run every format through the shared core. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 16 total unresolved issues (including 14 from previous reviews).
Reviewed by Cursor Bugbot for commit f2552df. Configure here.
| return | ||
| } | ||
| kind = kinds[code.Text(node, source)] | ||
| }) |
There was a problem hiding this comment.
Qualified parameter types miss client kinds
Medium Severity
typeScriptParameterKind only matches a single type_identifier or identifier against the import map. Namespace imports bind dotted names such as OpenAI.OpenAI, so a parameter typed as OpenAI.OpenAI or openai.OpenAI never receives a client kind. Constructor parameter properties and typed parameters then stay untracked, and later this.client.chat.completions.create(...) or client.messages.create(...) calls are not collected. Default and named type imports still work; import * as does not.
Reviewed by Cursor Bugbot for commit f2552df. Configure here.
| record(typeScriptInstanceID(node, pattern, source), kind) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Parameter properties skip constructor locals
Medium Severity
A TypeScript parameter property is recorded only as an instance field, then the walk returns without also binding the constructor parameter. this.client in methods works, but a use of the parameter name inside the constructor, such as client.chat.completions.create(...), does not see the client kind. Ordinary function parameters still bind in their function, so constructor parameter properties behave differently from every other typed parameter.
Reviewed by Cursor Bugbot for commit f2552df. Configure here.


Context
TypeScript and JavaScript sources need the same static rules as Python, and every format needs one conformance run.
What changes
Design notes
Review focus
Verification
make buildgo test ./...golangci-lint run ./...Related changes
🤖 Generated with Claude Code
Note
Overview
Adds
ecmascriptadapters fortypescriptandjavascriptthat statically parse sources with tree-sitter, resolve imports and literals, and expose agents, prompts, tools, and constants for sync—without executing code. Dynamic templates, spreads, mutations, and unresolved imports are marked read-only; dollar placeholders are translated where frameworks require it.Introduces
adaptertesthelpers (fixtures, edit application with byte checks, projection utilities) and packageall, which blank-imports every shipped format so hosts register them via side effect.Adds a broad
alltest suite: format conformance (read → update → reparse), framework matrix coverage for Python and TS/JS, markdown/agent-skill cases, structured-format edge cases, reference validation fuzzing, and new testdata fixtures for frameworks, imports, LangChain, and MCP servers.Reviewed by Cursor Bugbot for commit f2552df. Bugbot is set up for automated code reviews on this repo. Configure here.